refactor: use shared repo-cache for repository checkouts - #99
Conversation
9b2b98f to
e9b35a1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
registry_repository is now used to construct cache filesystem paths without strict validation, enabling potential path traversal outside the intended cache root.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors the repository overview collector to use SCORE’s shared repo_cache component for GitHub repository checkout synchronization, unifying cache layout and behavior (including empty-repo handling) across SCORE tooling.
Changes:
- Replace local clone/fetch/cleanup logic with
repo_cache(sync_default_branch) and align checkout paths to the canonical cache directory. - Update collector logic and tests to use repository identities (
full_name) and to forward credentials torepo_cacheviaGH_TOKEN. - Refresh documentation and dependency/lockfile metadata to include the new
repo-cachegit dependency.
File summaries
| File | Description |
|---|---|
| uv.lock | Adds repo-cache dependency pinned to a specific revision. |
| pyproject.toml | Introduces direct git dependency on repo-cache and enables direct references for Hatch metadata. |
| src/generate_repo_overview/constants.py | Switches default checkout base path to repo_cache.default_cache_directory(). |
| src/generate_repo_overview/collector/git_checkout.py | Delegates checkout sync to repo_cache.sync_default_branch; retains helper functions for reading refs/paths. |
| src/generate_repo_overview/collector/init.py | Sets/restores GH_TOKEN during collection so repo_cache subprocesses authenticate consistently. |
| src/generate_repo_overview/collector/repo_entry.py | Updates checkout identity/pathing to use full_name and removes redundant token plumbing for sync. |
| src/generate_repo_overview/collector/registry_metadata.py | Moves Bazel registry checkout into the shared cache layout and removes local checkout constant. |
| src/generate_repo_overview/collector/reference_integration.py | Uses shared cache layout for reference-integration and registry metadata discovery. |
| tests/test_git_checkout.py | Updates tests to validate delegation to repo_cache and preserves best-effort failure contract. |
| tests/test_repo_overview.py | Adjusts tests for new checkout sync behavior and verifies GH_TOKEN environment handling. |
| docs/repo-overview/usage-and-extension.md | Documents repo_cache/gh-based checkout synchronization and token forwarding. |
| docs/repo-overview/collection-and-cache.md | Updates cache layout and authentication details to match repo_cache. |
| docs/repo-overview/architecture.md | Updates architecture description to reflect delegation to repo_cache. |
| .gitignore | Simplifies cache ignore rules under profile/cache/. |
Review details
- Files reviewed: 12/14 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| repositories_by_module: dict[str, str] = {} | ||
| for metadata_path in sorted( | ||
| BAZEL_REGISTRY_LOCAL_CHECKOUT.glob("modules/*/metadata.json") | ||
| (default_cache_directory() / registry_repository).glob( | ||
| "modules/*/metadata.json" | ||
| ) |
There was a problem hiding this comment.
Thanks, this is correct. The previous validation only required a slash, so values such as ../etc, absolute paths, and Windows-style paths could escape the cache root when joined with default_cache_directory(). Commit 727a3f9 now requires exactly two non-empty repository path components and rejects dot/dotdot, rooted/drive paths, and backslashes both during configuration loading and defensively in the metadata lookup. Regression tests cover these cases; the full suite passes with 171 tests.
There was a problem hiding this comment.
🔵 Needs a closer look
Registry metadata lookup can use a different cache key from the resolved checkout and silently omit mappings.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/generate_repo_overview/collector/init.py:405
fetch_bazel_registry_metadata_by_repostores the checkout under the resolvedrepository.full_name(registry_metadata.py:28-37), but this passes the raw config value when reading it back. If GitHub canonicalizes the name (for example, casing or a repository rename redirect), the paths differ and registry-backed reference-integration mappings are silently omitted. Use the resolved full name when available so both operations share the same cache key.
- Files reviewed: 14/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Confirmed correct. The registry checkout is stored using the resolved repository object full_name, while the reference-integration lookup previously used the raw configuration value; those keys can differ after GitHub canonicalization or redirects. Commit 2090c3e now forwards the resolved full_name when available, falling back to the config value only if it is unavailable. Added regression coverage for the mismatch. The full suite passes with 172 tests. |
What this achieves
The repository overview collector now shares repository checkout synchronization and caching with SCORE’s
repo_cachecomponent. All repository checkouts, including registry, reference-integration, and platform repositories, use the same canonical cache layout. Empty repositories are handled as an expected state by the sharedrepo_cacheimplementation.Why this is necessary
The collector previously maintained its own clone, fetch, cleanup, and authentication plumbing. That duplicated behavior already needed by other SCORE tools and made checkout behavior inconsistent. Using the shared implementation keeps synchronization and empty-repository handling consistent across SCORE tools.
Changes
repo_cache.repo_cacherevision from tools main.